Skip to content

Enforce python import ordering rules. NFC #25594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 20, 2025
Merged

Enforce python import ordering rules. NFC #25594

merged 1 commit into from
Oct 20, 2025
+504 −315

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 19, 2025 •
edited
Loading

This change was automatically generated using ruff check --fix

@sbc100 sbc100 requested review from dschuff and kripken October 19, 2025 19:04
@sbc100 sbc100 enabled auto-merge (squash) October 20, 2025 14:48
kripken
kripken reviewed Oct 20, 2025
View reviewed changes
emrun.py
from BaseHTTPServer import HTTPServer
from SimpleHTTPServer import SimpleHTTPRequestHandler
from urllib import unquote
from urlparse import urlsplit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the rule here, do you know? the new version seems... arbitrary at first glance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about this one.. the rule is generally that things should be alphabetical and that system libraries always come first.

My guess is that because urlparse was removed in python3 and this code is supposed run on python2 that maybe the formatter is confused about local vs system modules here. I think I can just revert this file and mark it with an exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we could just remove python2 support from this file at this point I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted this file.

kripken
kripken approved these changes Oct 20, 2025
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok lgtm otherwise, the other changes do look good.

@sbc100 sbc100 merged commit 63c98e3 into emscripten-core:main Oct 20, 2025
34 checks passed
@sbc100 sbc100 deleted the import_order branch October 20, 2025 16:25
@kripken
Copy link
Member

kripken commented Oct 20, 2025

Oh, I missed the auto-merge... anyhow, it's ok as-is.

sbc100 added a commit that referenced this pull request Oct 20, 2025
@sbc100
Fix import order in update_settings_docs.py. NFC
dc92afc 
This was broken due to the #25594 and #25596 racing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kripken kripken kripken approved these changes

@dschuff dschuff Awaiting requested review from dschuff

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sbc100 @kripken